<fix>[multi]: batch guard NPE quality issues#3378
Open
zstack-robot-1 wants to merge 1 commit into5.5.6from
Open
<fix>[multi]: batch guard NPE quality issues#3378zstack-robot-1 wants to merge 1 commit into5.5.6from
zstack-robot-1 wants to merge 1 commit into5.5.6from
Conversation
- UpdateQueryImpl: guard val.getClass() NPE - LogSafeGson: return JsonNull when input is null - HostAllocatorChain: null check completion - VmInstanceVO: use Objects.equals to avoid NPE - SessionManagerImpl: guard null session - VmCapabilitiesJudger: guard null PS type result - CephPSMonBase: guard null self when evicted - CephPSBase: guard null mon.getSelf() - HostBase: guard null self when HostVO deleted - ExternalPSFactory: guard null URI protocol - LocalStorageBase: guard null errorCode.getCause() Resolves: ZSTAC-69300, ZSTAC-69957, ZSTAC-71973, ZSTAC-81294, ZSTAC-70180, ZSTAC-70181, ZSTAC-78309, ZSTAC-78310, ZSTAC-70668, ZSTAC-71909, ZSTAC-80555, ZSTAC-81270, ZSTAC-70101, ZSTAC-72034, ZSTAC-73197, ZSTAC-79921, ZSTAC-81160, ZSTAC-81224, ZSTAC-81805, ZSTAC-72304, ZSTAC-81804, ZSTAC-74898, ZSTAC-69215, ZSTAC-70151, ZSTAC-68933 Change-Id: I910e9b542ecd254fdf7e956f943316988a56a1f9
总览该拉取请求在多个 Java 文件中引入了防御性空指针安全检查，包括空值参数验证、空值条件判断和空值回退处理。这些更改通过提前检测和处理潜在的 null 值来增强代码的稳健性，防止 NullPointerException 异常。 变更
代码审查工作量估计🎯 2 (Simple) | ⏱️ ~12 分钟 诗歌
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.5)plugin/ceph/src/main/java/org/zstack/storage/ceph/primary/CephPrimaryStorageBase.javaComment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageAllocatorFactory.java (1)
388-393: 空指针防护逻辑正确，但错误码复用可能导致排查困难。这里对
parser的空检查是正确的防御性编程实践。但ORG_ZSTACK_STORAGE_PRIMARY_LOCAL_10023错误码在第 386 行已用于 "invalid uri" 场景，现在又用于 "unsupported protocol" 场景。两种不同的错误原因共用同一错误码，在日志排查时可能造成混淆。建议为"不支持的协议"场景分配独立的错误码，以便于问题定位。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageAllocatorFactory.java` around lines 388 - 393, The code in LocalStorageAllocatorFactory checks parser == null correctly but reuses ORG_ZSTACK_STORAGE_PRIMARY_LOCAL_10023 (already used for "invalid uri"), which makes diagnostics confusing; define a new distinct error code constant (e.g. ORG_ZSTACK_STORAGE_PRIMARY_LOCAL_10024 or similarly named) for the "unsupported protocol" case and replace the thrown OperationFailureException that currently references ORG_ZSTACK_STORAGE_PRIMARY_LOCAL_10023 with the new constant while keeping the same argerr message and the use of parser.parseUri(...).hostUuid to preserve behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@compute/src/main/java/org/zstack/compute/allocator/HostAllocatorChain.java`:
- Around line 149-152: The catch block in HostAllocatorChain swallows exceptions
when completion is null (dryRun), causing callers to hang; update the handler so
that if completion != null it calls
completion.fail(inerr(ORG_ZSTACK_COMPUTE_ALLOCATOR_10019, errMsg)), but if
completion == null (dryRun) rethrow a runtime exception (e.g., new
CloudRuntimeException or similar) created with
inerr(ORG_ZSTACK_COMPUTE_ALLOCATOR_10019, errMsg) or wrap the original Throwable
as the cause, ensuring the error surfaces to the caller instead of being
swallowed.
---
Nitpick comments:
In
`@plugin/localstorage/src/main/java/org/zstack/storage/primary/local/LocalStorageAllocatorFactory.java`:
- Around line 388-393: The code in LocalStorageAllocatorFactory checks parser ==
null correctly but reuses ORG_ZSTACK_STORAGE_PRIMARY_LOCAL_10023 (already used
for "invalid uri"), which makes diagnostics confusing; define a new distinct
error code constant (e.g. ORG_ZSTACK_STORAGE_PRIMARY_LOCAL_10024 or similarly
named) for the "unsupported protocol" case and replace the thrown
OperationFailureException that currently references
ORG_ZSTACK_STORAGE_PRIMARY_LOCAL_10023 with the new constant while keeping the
same argerr message and the use of parser.parseUri(...).hostUuid to preserve
behavior.
Comment on lines
+149
to
+152
| String errMsg = t != null ? t.toString() : "unknown error"; | ||
| if (completion != null) { | ||
| completion.fail(inerr(ORG_ZSTACK_COMPUTE_ALLOCATOR_10019, errMsg)); | ||
| } |
There was a problem hiding this comment.
dryRun 异常时未回调，可能导致调用方挂起。
dryRun() 场景下 completion 为空，当前 catch 只检查 completion，会吞掉异常导致调用方永远收不到失败回调。
🛠️ 建议修复
- String errMsg = t != null ? t.toString() : "unknown error";
- if (completion != null) {
- completion.fail(inerr(ORG_ZSTACK_COMPUTE_ALLOCATOR_10019, errMsg));
- }
+ String errMsg = t != null ? t.toString() : "unknown error";
+ if (isDryRun) {
+ if (dryRunCompletion != null) {
+ dryRunCompletion.fail(inerr(ORG_ZSTACK_COMPUTE_ALLOCATOR_10019, errMsg));
+ }
+ } else if (completion != null) {
+ completion.fail(inerr(ORG_ZSTACK_COMPUTE_ALLOCATOR_10019, errMsg));
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compute/src/main/java/org/zstack/compute/allocator/HostAllocatorChain.java`
around lines 149 - 152, The catch block in HostAllocatorChain swallows
exceptions when completion is null (dryRun), causing callers to hang; update the
handler so that if completion != null it calls
completion.fail(inerr(ORG_ZSTACK_COMPUTE_ALLOCATOR_10019, errMsg)), but if
completion == null (dryRun) rethrow a runtime exception (e.g., new
CloudRuntimeException or similar) created with
inerr(ORG_ZSTACK_COMPUTE_ALLOCATOR_10019, errMsg) or wrap the original Throwable
as the cause, ensuring the error surfaces to the caller instead of being
swallowed.
Collaborator
|
Comment on Comment from ye.zou: Fixed in fd02d47e3c — now routes to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Batch fix for 25 NPE issues found by periodic quality scripts.
Resolves
ZSTAC-69300, ZSTAC-69957, ZSTAC-71973, ZSTAC-81294, ZSTAC-70180, ZSTAC-70181, ZSTAC-78309, ZSTAC-78310, ZSTAC-70668, ZSTAC-71909, ZSTAC-80555, ZSTAC-81270, ZSTAC-70101, ZSTAC-72034, ZSTAC-73197, ZSTAC-79921, ZSTAC-81160, ZSTAC-81224 + more
Testing
mvn compile -pl affected-modules -am -Dmaven.test.skipsync from gitlab !9213